Refactor/rename messaging contribution push param#10996
Refactor/rename messaging contribution push param#10996harbin1053020115 wants to merge 5 commits intoeclipse-theia:masterfrom
Conversation
vince-fugnitto
left a comment
There was a problem hiding this comment.
Thank you for your contribution 👍
In order to accept your changes please be sure to sure to sign the eclipse contributor agreement (eca) with the same email as your authorship.
I'm not sure I fully understand the reasons for the change, it is only updating the variable name in the callback, uses of MessagingContribution.push still use channel (verified with find all references).
The original |
I believe |
|
so, if this is meaningful: push(spec: string, callback: (params: MessagingService.PathParams, connection: T) => void): void {
const route = new Route(spec);
// here is the change, or you can look at the lastest changes.
this.handlers.push((path, channel: T) => {
const params = route.match(path);
if (!params) {
return false;
}
callback(params, channel);
return route.reverse(params);
});
} |
| push(spec: string, callback: (params: MessagingService.PathParams, connection: T) => void): void { | ||
| const route = new Route(spec); | ||
| this.handlers.push((path, channel) => { | ||
| this.handlers.push((path, channel: T) => { |
There was a problem hiding this comment.
I don't see the value in this change?
We can already infer the type from handlers:
protected readonly handlers: ((path: string, connection: T) => string | false)[] = [];and the editor gives us hover information that channel is of type T.
What exactly is the change attempting to achieve?
There was a problem hiding this comment.
I see, please ignore this pr
There was a problem hiding this comment.
No problem! If there are improvements or features you'd like to work on please don't hesitate to contribute and we'd be happy to review them :)
What it does
Rename message-contirbution.ts(packages/core/src/node/messaging/messaging-contribution.ts) ConnectionHandlers.push param, which is changing channel to socket.
How to test
No need to test.
Review checklist
Reminder for reviewers